-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support OIDC token propagation during SecurityIdentity augmentation #34933
Support OIDC token propagation during SecurityIdentity augmentation #34933
Conversation
656369b
to
4eac45b
Compare
Thanks @michalvavrik for giving it a try, will have a look soon, cheers |
This comment has been minimized.
This comment has been minimized.
I confirm it's the proper approach to propagate data along async (and sync) processing. Note that you can also use the request scope which is written in the duplicated context if available. BTW, a lot of extensions are using the duplicated context without knowing about it. About the email it's because you must not write data in the root context (shared by concurrent requests) and only in the duplicated context. No worries if you try to do that in Quarkus, it bites... (well, it just throws an exception). So if you were able to write in the context, it's fine. |
I just put breakpoint there, request scope is not yet written in the duplicated context. This code is run during authentication when proactive auth is enabled.
Thank you very much! |
FYI @sberyozkin: when testing Clement suggestion I realized previous solution didn't work for disabled proactive auth when Arc request context is already active, however we can't use for accessing SecurityIdentity (token credential that comes from it) it as it is circular reference; so I pushed slight adjustment. |
4eac45b
to
a7a6033
Compare
This comment has been minimized.
This comment has been minimized.
Hi Michal thanks for the PR and Clement for your help as well, It does look like a technically precise PR, I'm a little bit uncertain if we really want to do it though, given a number of caveats. Let me clarify. The use case is, use the current token in But this is a very specific case requiring a specific client interface support - not an application level rest client interface which endpoints will use themselves. So users can type in an interface with the This is why I'm a little bit uncertain, given the complexities/caveats involved, if we want to generalize it around this specific use case which can be easily solved with alternative means. Michal, as always I appreciate very much your help :-), I'm just having some doubts :-). |
Just FYI: when I was developer few years back, using Keycloak as SSO, we had exactly this use case when microservice needed to access SAP database with user information and roles. I presume this is the edge because noone opened issue about this till now, but common sense suggest that is is not. Especially if you Keycloak user federation between organisations, you will have SSO for auth, but in order to avoid buttleneck, you might want to have other database specific to application / department for authorization that can be accessed without IT engineers (for example: SAP). Full disclosure: I don't really know how users are using Quarkus OIDC, you will have much better idea. I just meant to let you know this is not unseen use case, that's all.
Yes, I agree there is a workaround, but this code that I added is only executed in scenarios where access token propagation is enabled and actually used inside authentication. Is you set
One question: I don't believe this is just about propagating headers as when exchange token is enabled, filter also exchanges tokens. How will users also do that, or this is not related? Line 76 in 2da3206
This is absolutely fine. I've learned something about context & token propagation, so if this ends up to be abandoned, I'm still happy. I came with only solution I can see. If you prefer to either:
no problem at all. UPDATE: I thought about it and maybe you meant this is a whole lot of a code for something we don't need to solve for user. Did I get it right? |
Hey @michalvavrik , as far as the token exchange is concerned, it is about managing audience and token targets correctly in multiple hop propagation scenarios. I understand the use case itself, sure. I wonder, can we somehow reformat this PR not to focus on token propagation only, example, users have often requested Routing Context injected in the augmentor ? |
We are adding routing context to auth request that is passed to None of that would fix linked issue though. There is no need to stretch it, I suggest that I will close this one. Instead of putting There is also possible partial solution - we could fix access token propagation filter fix for disabled proactive auth as there we can inject I'll let PR open for day or two in case you have some idea and then close it. No need to spend more time on this. Thanks! |
@michalvavrik Hi, please don't close PR yet, let it be open for a few days at least. I'll comment more as well. Thanks |
:-) In that case I'll add one more note. I didn't emphasize it before because I think the gist of your doubts is that this is change made for the edge case and whether we should add code that will only be used for this case. This solution should be safe, because propagating data through duplicated context when safe toggle is set to true should be safe according to Clement comment and what I read. IIUC there is effort for other extensions to use duplicated context. |
.../io/quarkus/oidc/token/propagation/reactive/OidcTokenPropagationReactiveBuildTimeConfig.java
Outdated
Show resolved
Hide resolved
.../oidc/runtime/src/main/java/io/quarkus/oidc/runtime/AbstractOidcAuthenticationMechanism.java
Outdated
Show resolved
Hide resolved
...n/java/io/quarkus/oidc/token/propagation/reactive/OidcTokenPropagationReactiveBuildStep.java
Show resolved
Hide resolved
...n/java/io/quarkus/oidc/token/propagation/reactive/OidcTokenPropagationReactiveBuildStep.java
Outdated
Show resolved
Hide resolved
.../io/quarkus/oidc/token/propagation/reactive/OidcTokenPropagationReactiveBuildTimeConfig.java
Outdated
Show resolved
Hide resolved
@michalvavrik Can you check the review comments please when you get a chance ? |
I mentioned you commented (thanks btw) and I'll address them tomorrow. I was rather busy today :-) |
a7a6033
to
e215fcb
Compare
e215fcb
to
eaf5324
Compare
0a591a8
to
7e7b523
Compare
This comment has been minimized.
This comment has been minimized.
Failure in native is related somehow, I think it is also related to |
7e7b523
to
9e79813
Compare
Added test for SmallRye JWT and fixed PR so that it work properly in native mode. |
This comment has been minimized.
This comment has been minimized.
extensions/oidc-common/runtime/src/main/java/io/quarkus/oidc/common/runtime/OidcConstants.java
Outdated
Show resolved
Hide resolved
9e79813
to
7f1ccee
Compare
This comment has been minimized.
This comment has been minimized.
...lient/runtime/src/main/java/io/quarkus/oidc/token/propagation/TokenPropagationConstants.java
Show resolved
Hide resolved
.../io/quarkus/oidc/token/propagation/reactive/OidcTokenPropagationReactiveBuildTimeConfig.java
Outdated
Show resolved
Hide resolved
...main/java/io/quarkus/oidc/token/propagation/runtime/OidcTokenPropagationBuildTimeConfig.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks, should be good to go after minor comments related to JavaDocs get addressed
7f1ccee
to
202addd
Compare
✖ This workflow run has failed but no jobs reported an error. Something weird happened, please check the workflow run page carefully: it might be an issue with the workflow configuration itself. |
fixes: #33994
Currently you can't use
AccessTokenRequest
filter on the REST client during security identity augmentation as:io.quarkus.vertx.http.runtime.CurrentVertxRequest
had not been set yet etc.Only way I actually found was putting local data to duplicated context. Reproducer I created always had context marked as safe, I didn't find any core extension that marks context as unsafe. Only extensions that I found which are using duplicated context local data are Hibernate Reactive, Mongo Panache Reactive, Hibernate Reactive Panache, OpenTelemetry and Micrometer. I understand this is not preferrable way to propagate context data. I also found this answer from @cescoffier https://stackoverflow.com/questions/71242590/can-i-store-sensitive-data-in-a-vert-x-context-in-a-quarkus-application that made me more optimistic that it is safe to use duplicated context when available. And https://groups.google.com/g/quarkus-dev/c/lBmQkCi_VK0/m/_O9b9mUxBQAJ made me less optimistic.
This PR propose enabling token propagation during auth only when explicitly enabled and with caveats.